Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check full stream equality when deduplicating #304

Merged
merged 2 commits into from
May 22, 2019

Conversation

sgallagher
Copy link
Collaborator

Previously, we assumed that any two streams having the same NSVCA
were identical. Now we can actually test for full equality before
dropping one while deduplicating. Note that this will result in a
hard failure if they aren't identical, because this means that the
enabled repositories are providing conflicting data.

Fixes: #188

Signed-off-by: Stephen Gallagher [email protected]

@Conan-Kudo Can I get your opinion on whether it's acceptable to treat this as a hard failure? In the unlikely event that two repositories (or the same repository) are providing two modulemd documents with the same NSVCA but different content elsewhere, I don't think there's any fail-safe way to pick one over another, so I think our only option is to return an error.

Also includes a dependent fix being tracked and reviewed in #303

Comparison of the XMDs were accidentally returning the opposite
truth value.

Signed-off-by: Stephen Gallagher <[email protected]>
Previously, we assumed that any two streams having the same NSVCA
were identical. Now we can actually test for full equality before
dropping one while deduplicating. Note that this will result in a
hard failure if they aren't identical, because this means that the
enabled repositories are providing conflicting data.

Fixes: fedora-modularity#188

Signed-off-by: Stephen Gallagher <[email protected]>
@sgallagher sgallagher requested a review from Conan-Kudo May 22, 2019 00:37
@sgallagher sgallagher mentioned this pull request May 22, 2019
@sgallagher sgallagher modified the milestones: 2.4.1, 2.5.0 May 22, 2019
@Conan-Kudo
Copy link
Collaborator

@sgallagher My understanding is that the behavior you want to break is why we're even using YAML in the first place? The free-form merging to allow a module to grow with multiple sources was one of the points brought up against changing the output format to XML.

What changed your mind about disabling that behavior?

@sgallagher
Copy link
Collaborator Author

@Conan-Kudo I think you misunderstand. The NSVCA (module_name:stream_name:version:context:architecture) must be unique for the YAML. If you want to extend the module you can change any one of those fields to modify. A different stream name, version or context won't conflict.

The problem is if you have two different repos both claiming to define nodejs:12:20190522120000:deadbeef:x86_64 but not providing the same content. In that case, there's really no way I can think of to decide which one gets to own that NSVCA cleanly.

Copy link
Collaborator

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, and makes it easier to reason modules. However, now I want XML modulemd for repodata again... :)

@Conan-Kudo
Copy link
Collaborator

@sgallagher It was previously explained to me that it was intentionally possible to do that, and that the intent was that the documents would be merged on the fly to create a module superset (which is incredibly difficult to reason out!) that could be installed with content from both repositories.

@sgallagher
Copy link
Collaborator Author

sgallagher commented May 22, 2019

@Conan-Kudo Who said that to you? This has never been true (or intended) so far as I'm aware.

@Conan-Kudo
Copy link
Collaborator

@sgallagher You and @contyk two months ago.

@sgallagher
Copy link
Collaborator Author

@Conan-Kudo It's possible that you misunderstood when I was talking about merging content into the ModuleIndex. It is supported that different streams might come from different repos, but never that the YAML for a specific NSVCA would come from two different places. That would be madness!

@sgallagher sgallagher merged commit 10e0db0 into fedora-modularity:master May 22, 2019
@sgallagher sgallagher deleted the stream_equals branch July 15, 2019 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance IndexMerger to use equality checks
2 participants